Skip to content

Add Descriptor::iter_pk - #823

Merged
apoelstra merged 2 commits into
masterfrom
push-slnpvpouumpw
Jun 12, 2025
Merged

apoelstra merged 2 commits into
masterfrom
push-slnpvpouumpw

Conversation

@apoelstra

Copy link
Copy Markdown
Member

Fixes #821.

Should backport to 12.x.

This commit was -not- AI-generated, though probably it should've been.
@apoelstra
apoelstra force-pushed the push-slnpvpouumpw branch 2 times, most recently from d3fab75 to 82f86e1 Compare May 23, 2025 23:31
This one -was- AI-generated, though I reviewed it.
@apoelstra
apoelstra force-pushed the push-slnpvpouumpw branch from 82f86e1 to aa42cd3 Compare May 23, 2025 23:36

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On aa42cd3 successfully ran local tests

@sanket1729 sanket1729 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question. Rest LGTM

Comment thread src/descriptor/iter.rs
use crate::{miniscript, Miniscript, MiniscriptKey};

/// Iterator over all the keys in a descriptor.
pub struct PkIter<'desc, Pk: MiniscriptKey> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. But Ideally, we want this to be a sum type instead of a product type? But then we get whole loads of matches everywhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason to use a product type was that Taproot descriptors have both single keys and (optionally) a taptree iterator.

And in PkIter::next I have a construction where the taptree iterator yields new Miniscript pkiters.

I can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.

Yeah, that would be messy. Ideally, we want to stick the mantra of "make invalid states unrepresentable"; but it might be overkill for this small change

@sanket1729 sanket1729 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK aa42cd3

Comment thread src/descriptor/iter.rs
use crate::{miniscript, Miniscript, MiniscriptKey};

/// Iterator over all the keys in a descriptor.
pub struct PkIter<'desc, Pk: MiniscriptKey> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to refactor this to use a sum type but I think it would result in more repeated/redundant code.

Yeah, that would be messy. Ideally, we want to stick the mantra of "make invalid states unrepresentable"; but it might be overkill for this small change

@apoelstra

Copy link
Copy Markdown
Member Author

Let's revisit this after we get rid of the Ctx object on Miniscript. I think that parameter is causing a lot of the ugly code.

@apoelstra
apoelstra merged commit 9a9f9b1 into master Jun 12, 2025
@apoelstra
apoelstra deleted the push-slnpvpouumpw branch June 12, 2025 22:10
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
aa42cd390a700384902fb932c63c308ee0486272 descriptor: add unit tests for iter_pk (Andrew Poelstra)
523654e78e6ef093211fadf02cbd96823d169801 descriptor: add Descriptor::iter_pk (Andrew Poelstra)

Pull request description:

  Fixes #821.

  Should backport to 12.x.

ACKs for top commit:
  sanket1729:
    ACK aa42cd390a700384902fb932c63c308ee0486272

Tree-SHA512: 15a5aa6f6d0d7ea5773cdecd4a67320ee89027eaf178c44ad7e1d1465752eb95d835c3dcf1dbb734302e826deeb13803178c7999e15bce710c5fd4dc31c876d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Descriptor::iter_pk()

2 participants